#46744 Prevent the default role from being set to a privileged role when user registration is open - #12977
#46744 Prevent the default role from being set to a privileged role when user registration is open#12977johnbillion wants to merge 5 commits into
Conversation
…r registration is open.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
audrasjb
left a comment
There was a problem hiding this comment.
The logic looks good to me.
I'm quite fine with the filter_default_role function name as long as it is not used at all (at least no occurrence exists in the wpdirectory.net database yet), but we definitely need to add a docblock before the function, event if it basically just show the same content used below in the hook.
There was a problem hiding this comment.
Pull request overview
This PR hardens WordPress user registration by preventing the default_role option from effectively resolving to a privileged role when user registration is enabled, while also providing a new filter (default_role_excluded_roles) to customize which roles are disallowed.
Changes:
- Adds an
option_default_rolefilter to override privileged default roles tosubscriberwhen registration is open. - Introduces the
default_role_excluded_rolesfilter (defaulting toadministratorandeditor) and wires it into the General Settings role dropdown behavior. - Adds PHPUnit coverage for both direct filtering behavior and end-to-end user creation role assignment.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/phpunit/tests/functions/filterDefaultRole.php | Adds unit/integration tests covering filtering behavior and user creation role assignment. |
| src/wp-includes/functions.php | Introduces the filter_default_role() option filter callback and the default_role_excluded_roles filter. |
| src/wp-includes/default-filters.php | Hooks the new default-role filter callback to option_default_role. |
| src/wp-admin/options-general.php | Uses default_role_excluded_roles to seed the excluded roles used in the default role dropdown logic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * @param string $default_role The default role for new user registrations. | ||
| * @return string The filtered default role for new user registrations. | ||
| */ | ||
| function filter_default_role( $default_role ) { |
audrasjb
left a comment
There was a problem hiding this comment.
This looks good to me 👍
| * @return string The filtered default role for new user registrations. | ||
| */ | ||
| function filter_default_role( $default_role ) { | ||
| static $filtering = false; |
There was a problem hiding this comment.
Why Is there a static variable in use here? It feels unnecessary
whyisjake
left a comment
There was a problem hiding this comment.
Notes from a pre-commit read. The first one is structural and I think it's worth settling before this lands; the rest are smaller.
AI assistance: Yes. Tool(s): Claude Code. Used for: reviewing the diff against trunk and mechanically verifying the update_option() and Site Health interactions described below.
| add_filter( 'option_blog_charset', '_canonical_charset' ); | ||
| add_filter( 'option_home', '_config_wp_home' ); | ||
| add_filter( 'option_siteurl', '_config_wp_siteurl' ); | ||
| add_filter( 'option_default_role', 'filter_default_role' ); |
There was a problem hiding this comment.
Filtering the option on read rather than clamping it on write has three consequences downstream that I don't think are intended. I verified each against trunk:
1. The companion Site Health check can no longer fire. WP_Site_Health::get_test_insecure_registration() — added in 7.0.0 for this same ticket — does:
if ( $users_can_register && in_array( $default_role, array( 'editor', 'administrator' ), true ) ) {It reads get_option( 'default_role' ), which is now filtered. Its critical branch requires users_can_register to be truthy, which is exactly the condition under which filter_default_role() has already rewritten the value to subscriber. So the branch becomes unreachable: the one screen that warned owners about this configuration reports "good" while the database still holds administrator.
2. An admin can no longer save a corrected value. update_option() at option.php:887 does $old_value = get_option( $option ); — the filtered value — and then returns early:
if ( $value === $old_value || maybe_serialize( $value ) === maybe_serialize( $old_value ) ) {
return false;
}With administrator stored and registration open, get_option() returns subscriber, so selecting "Subscriber" in Settings > General writes nothing. "Settings saved", database unchanged. The privileged value stays until someone turns registration off, at which point the clamp lifts and the site silently resumes creating administrators.
3. WP_Roles::remove_role() breaks on the same comparison — class-wp-roles.php:217 uses if ( get_option( 'default_role' ) === $role ) to reset the default when the role is removed, and that can no longer match the stored value.
All three go away if enforcement moves to write time. sanitize_option() already has a default_role case at formatting.php:5165 handling the missing-role fallback, which looks like the natural home; pre_update_option_default_role would also work. Stored and effective values then agree, Site Health keeps working, and the settings screen stops disagreeing with the database.
| * | ||
| * @param string[] $roles Roles that are excluded from being available. | ||
| */ | ||
| $excluded = apply_filters( 'default_role_excluded_roles', array( 'administrator', 'editor' ) ); |
There was a problem hiding this comment.
A slug list means a custom role carrying manage_options, promote_users, edit_users or edit_plugins passes straight through — and with the read-filter approach, Site Health now reports "good" for it too. Administrator clones from role-editor, membership and LMS plugins are exactly the population that ends up here.
A capability probe over wp_roles()->roles would produce the default set, still passed through default_role_excluded_roles so it stays adjustable. Worth checking wp_roles() availability at this point in the bootstrap first, since the filter is registered from default-filters.php.
Separately on the hook itself: it's currently removable, and the PR's own test_excluded_roles_can_be_removed uses __return_empty_array to restore administrator as the default role. If the intent is a floor rather than a suggestion, array_unique( array_merge( array( 'administrator', 'editor' ), (array) apply_filters( ... ) ) ) keeps it additive. If a genuine opt-out is wanted, that's fine — but the docblock should say that emptying the list re-enables self-registration into privileged roles.
| return $default_role; | ||
| } | ||
|
|
||
| $filtering = true; |
There was a problem hiding this comment.
Following on from @aaronjorbin's question about whether this guard is needed — whatever the answer, it currently fails open.
$filtering = false; at 9454 is only reached on the normal path. get_option( 'users_can_register' ) and the default_role_excluded_roles filter both run third-party callbacks in between, so if any of them throws and something upstream catches it, $filtering stays true for the rest of the request and every subsequent call returns the raw stored value — including administrator.
A try/finally around the body fixes it, and the re-entrant short circuit at 9433 returning 'subscriber' rather than $default_role would make that path fail closed too.
No test covers this. Nothing in the suite exercises a nested or throwing call, so a broken guard would surface in production rather than CI.
| * @param string $default_role The default role for new user registrations. | ||
| * @return string The filtered default role for new user registrations. | ||
| */ | ||
| function filter_default_role( $default_role ) { |
There was a problem hiding this comment.
filter_default_role() is a new unprefixed global in wp-includes/functions.php. Core convention is wp_ or a leading underscore — _config_wp_home() and _mce_set_direction() nearby are the pattern.
This isn't only style. It's declared unconditionally and functions.php loads before plugins, so any plugin already defining a global filter_default_role() is a fatal "Cannot redeclare" on upgrade. That's the same shape as the wp_set_cookie() collision with WP Consent API on #12444, so a plugin-directory search on the final name seems worth doing before commit.
Also: @since 7.2.0 here versus 7.0.0 on the dropdown filter in options-general.php — worth a check that both are right for the release this actually lands in.
This change prevents the default role from containing a dangerous value when user registration is open. It does this via filtering the
default_roleoption value, meaning a privileged value in the option in the database gets overridden.The Editor and Administrator roles are excluded by default (when user registration is open). The
default_role_excluded_rolesfilter is introduced to facilitate adding to or removing from the list of excluded roles.On a Multisite installation, the
users_can_register_signup_filter()filter is applied tooption_users_can_register. This PR doesn't need to handle that specifically, but tests have been added to ensure it's covered.Trac ticket: Core-46744
Use of AI Tools
Changes written manually, tests written by Opus 5.